Clear suppressed notifications only after the TimePeriod is resumed #10613
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jschmidt-icinga
force-pushed
the
clear-suppr-notif-after-tp-resume
branch
from
a6ba476 to
20560af
Compare
Al2Klimov
previously approved these changes
Oct 21, 2025
|
|
||
| if ((suppressedTypes & type) && !checkable->NotificationReasonApplies(type)) { | ||
| subtract |= type; | ||
| suppressedTypes &= ~type; |
Member
There was a problem hiding this comment.
Didn't read the issue, but the fix shouldn't hurt. After all, this is also the code branch where notifications are sent.
jschmidt-icinga
force-pushed
the
clear-suppr-notif-after-tp-resume
branch
3 times, most recently
from
1fd58ee to
22d7a0d
Compare
jschmidt-icinga
force-pushed
the
clear-suppr-notif-after-tp-resume
branch
2 times, most recently
from
c08038d to
fea4554
Compare
jschmidt-icinga
force-pushed
the
clear-suppr-notif-after-tp-resume
branch
from
fea4554 to
9fa3acc
Compare
jschmidt-icinga
force-pushed
the
clear-suppr-notif-after-tp-resume
branch
from
9fa3acc to
41b5383
Compare
yhabteab
reviewed
Oct 28, 2025
Member
There was a problem hiding this comment.
I'm not entirely certain what the purpose of that check is, but the only scenario I could think of is when a
TimePeriodbegins and a check result produces a notification beforeNotificationTimerHandlercan run and send out the suppressed notifications.
That's pretty much the reasoning behind it. Its purpose is to avoid sending notifications that are no longer applicable. For instance, if a problem notification was suppressed due to being outside the time period, but recovers right after the time period starts and before the NotificationTimerHandler runs, it won't clear the suppressed problem notification, thus sending out the suppressed problem notification would be incorrect.
jschmidt-icinga
force-pushed
the
clear-suppr-notif-after-tp-resume
branch
5 times, most recently
from
1bb78fa to
393a267
Compare
yhabteab
reviewed
Nov 3, 2025
jschmidt-icinga
force-pushed
the
clear-suppr-notif-after-tp-resume
branch
from
393a267 to
2d32c6b
Compare
This includes a few common scenarios and a reproduction of the current behavior affected by the underlying bug of issue #10575. This is done both to document the change in behavior, as well as to ensure the behavior of the other scenarios stays the same before and after the fix is applied.
Without this commit, every time the NotificationTimerHandler runs it will discard notifications that don't apply to the reason of the latest check result. This is probably intended to clear outdated suppressed notifications immediately when the TimePeriod resumes, but it also clears out important ones (see the test case). This commit fixes that by clearing out inapplicable notifications when the timer runs the first time after the TimePeriod resumes. By that time we can expect that no new suppressed notifications will be added and all notifications that don't conflict with the last check-result can still be run. Fixes #10575
jschmidt-icinga
force-pushed
the
clear-suppr-notif-after-tp-resume
branch
from
2d32c6b to
75c7d28
Compare
yhabteab
approved these changes
Nov 3, 2025
yhabteab
added
bug
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Bug Description 🐞
When multiple state changes happen outside a
TimePeriod,NotificationComponent::FireSuppressedNotifications()will clear notifications that don't apply too soon, leading the state of the suppressed notifications to become inconsistent.icinga2/lib/notification/notificationcomponent.cpp
Lines 86 to 91 in 5d46ca4
Ultimately this will cause any suppressed notifications to be discarded when the
TimePeriodresumes.Fix Description 🔧
The trivial fix is to just move the check on
NotificationReasonApplies()inside thetp->IsInside()condition, thereby removing them after theTimePeriodresumes and leaving the list of suppressed notifications intact until no further notifications will be added to it.I'm not entirely certain what the purpose of that check is, but the only scenario I could think of is when a
TimePeriodbegins and a check result produces a notification beforeNotificationTimerHandlercan run and send out the suppressed notifications. I've added a unit-test to ensure that behavior is the same as before.@Al2Klimov, since you're the one who wrote the original code, maybe you can give other scenarios where this check might be affecting and that could potentially stop working with this PR. In that case we can add additional test-cases and look for a more complex fix.
Unit-Tests ✅
I've added tests for some common scenarios including the bugged behavior of the issue this PR fixes. This way the behavior of the
NotificationComponentcan be verified before and after this PR. I'm happy to add additional test-cases for behavior that might be affected by this issue and the fix.Fixes #10575